-
Notifications
You must be signed in to change notification settings - Fork 13.5k
add const_make_global
; err for const_allocate
ptrs if didn't call
#143595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
67537c4
to
911c6a6
Compare
I'm really excited about this experiment and the movement on const alloc, so thanks for working on it <3 |
This comment has been minimized.
This comment has been minimized.
In terms of naming, I think we said that The original plan I proposed also makes the allocation immutable when it gets marked as global -- basically, this operation should indicate that you are done preparing this allocation and it is ready to be interned. (We actually intern it later as that's easier, but for the API this does not matter.) |
aa4e8a9
to
a1ef719
Compare
alloc.mutability = Mutability::Not; | ||
let alloc = self.tcx.mk_const_alloc(alloc); | ||
self.tcx.set_alloc_id_memory(alloc_id, alloc); | ||
interp_ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This risks breaking key invariants like "tcx-managed allocations do not have any dangling pointers". So as nice as this is I think it won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure both ptr_get_alloc_id
and self.memory.alloc_map.remove(&alloc_id)
calls above ensure that this allocation is valid, unless you mean that the allocation itself could contain values that are dangling pointers to other stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit easier in this situation than during interning, because we don't need to intern nested allocations, so you could just iterate over the relocation table and check if all of the relocations in it are already interned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want that, as already mentioned it would make it impossible to use this on cyclic structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of only marking it as immutable here and leaving it for the interning to actually make it into global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for a cyclic globalification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for a cyclic globalification
This turned out to be the hardest part that I haven't found enough time to come up with an adequate change to address. See the following snippet:
#![feature(core_intrinsics)]
#![feature(const_heap)]
use std::mem::{align_of, size_of};
use std::intrinsics;
struct SelfReferential {
me: *const SelfReferential,
}
const Y: &'static SelfReferential = unsafe {
let size = size_of::<SelfReferential>();
let align = align_of::<SelfReferential>();
let ptr_raw = intrinsics::const_allocate(size, align);
let ptr = ptr_raw as *mut SelfReferential;
let me_addr = &raw mut (*ptr).me;
*me_addr = ptr;
intrinsics::const_make_global(ptr_raw, size, align);
&*ptr
};
fn main() {}
me
's provenance says it wasn't derived from an immutable reference. We can't really change that because it can't be mutated after we callconst_make_global
.- The fact above hits "accepted a mutable pointer that should not have accepted" at
rust/compiler/rustc_const_eval/src/interpret/intern.rs
Lines 247 to 265 in 040e2f8
// Ensure that this is derived from a shared reference. Crucially, we check this *before* // checking whether the `alloc_id` has already been interned. The point of this check is to // ensure that when there are multiple pointers to the same allocation, they are *all* // derived from a shared reference. Therefore it would be bad if we only checked the first // pointer to any given allocation. // (It is likely not possible to actually have multiple pointers to the same allocation, // so alternatively we could also check that and ICE if there are multiple such pointers.) // See <https://github.com/rust-lang/rust/pull/128543> for why we are checking for "shared // reference" and not "immutable", i.e., for why we are allowing interior-mutable shared // references: they can actually be created in safe code while pointing to apparently // "immutable" values, via promotion or tail expression lifetime extension of // `&None::<Cell<T>>`. // We also exclude promoteds from this as `&mut []` can be promoted, which is a mutable // reference pointing to an immutable (zero-sized) allocation. We rely on the promotion // analysis not screwing up to ensure that it is sound to intern promoteds as immutable. if intern_kind != InternKind::Promoted && inner_mutability == Mutability::Not && !prov.shared_ref() { - We can check if
alloc_id
has kindHeap { was_made_immut: true }
and skip the error that way. intern_shallow
will keep returning alloc2 adding to our todo list being weird left and right. I added a.filter
inrust/compiler/rustc_const_eval/src/interpret/intern.rs
Lines 311 to 312 in 040e2f8
match intern_shallow(ecx, alloc_id, inner_mutability, Some(&mut disambiguator)) { Ok(nested) => todo.extend(nested), just_interned
.- Apparently that wasn't enough and rustc hits an infinite loop at some point. The stack trace isn't helpful enough and I didn't have enough time to find a way to debug the stack overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we will probably have to relax that "derived from shared ref" check for heap allocations... which should be fine since those are all explicitly marked as immutable via make_global
so nobody can complain if it is UB to mutate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we leave the cyclic part to a next PR, seems easier to review this one that way. At least the API is done in a way that cyclic globalification should be possible.
575f024
to
b1cc426
Compare
We already have a name for this: static promotion. What do you think about reusing that terminology here? |
alloc.mutability = Mutability::Not; | ||
let alloc = self.tcx.mk_const_alloc(alloc); | ||
self.tcx.set_alloc_id_memory(alloc_id, alloc); | ||
interp_ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit easier in this situation than during interning, because we don't need to intern nested allocations, so you could just iterate over the relocation table and check if all of the relocations in it are already interned
So what would the API look like then, I think of static promotion as a MIR transformation. In that sense, it has very little to do with this PR, which is about achieving a similar result but via very different means. |
FWIW PR name, description, and commit message also still need updating for the new term. |
b1cc426
to
5badbfa
Compare
const_leak
and reject interning non-leaked const_allocate
ptrsconst_make_global
; err for const_allocate
ptrs if didn't call
This comment has been minimized.
This comment has been minimized.
That should do it, thanks. :) @bors r+ |
This PR changes a file inside |
@bors r=RalfJung,fee1-dead |
…r, r=RalfJung,fee1-dead add `const_make_global`; err for `const_allocate` ptrs if didn't call Implements as discussed on Zulip: [#t-compiler/const-eval > const heap](https://rust-lang.zulipchat.com/#narrow/channel/146212-t-compiler.2Fconst-eval/topic/const.20heap/with/527125421) r? `@rust-lang/wg-const-eval` Fixes rust-lang#129233
…r, r=RalfJung,fee1-dead add `const_make_global`; err for `const_allocate` ptrs if didn't call Implements as discussed on Zulip: [#t-compiler/const-eval > const heap](https://rust-lang.zulipchat.com/#narrow/channel/146212-t-compiler.2Fconst-eval/topic/const.20heap/with/527125421) r? ``@rust-lang/wg-const-eval`` Fixes rust-lang#129233
…r, r=RalfJung,fee1-dead add `const_make_global`; err for `const_allocate` ptrs if didn't call Implements as discussed on Zulip: [#t-compiler/const-eval > const heap](https://rust-lang.zulipchat.com/#narrow/channel/146212-t-compiler.2Fconst-eval/topic/const.20heap/with/527125421) r? ```@rust-lang/wg-const-eval``` Fixes rust-lang#129233
Rollup of 11 pull requests Successful merges: - #143595 (add `const_make_global`; err for `const_allocate` ptrs if didn't call) - #143678 (Added error for invalid char cast) - #143793 (Opaque type collection: Guard against endlessly recursing free alias types) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143829 (Trim `BorrowedCursor` API) - #143856 (Linting public reexport of private dependencies) - #143891 (Port `#[coverage]` to the new attribute system) - #143914 (Reword mismatched-lifetime-syntaxes text based on feedback) - #143922 (Improve path segment joining) - #143926 (Remove deprecated fields in bootstrap) - #143975 (type_id_eq: check that the hash fully matches the type) r? `@ghost` `@rustbot` modify labels: rollup
…r, r=RalfJung,fee1-dead add `const_make_global`; err for `const_allocate` ptrs if didn't call Implements as discussed on Zulip: [#t-compiler/const-eval > const heap](https://rust-lang.zulipchat.com/#narrow/channel/146212-t-compiler.2Fconst-eval/topic/const.20heap/with/527125421) r? ``@rust-lang/wg-const-eval`` Fixes rust-lang#129233
Rollup of 15 pull requests Successful merges: - #142304 (tests: Add `RUST_BACKTRACE` and `-Cpanic` revisions to `panic-main.rs` test) - #143388 (Various refactors to the LTO handling code) - #143409 (Enable xgot feature for mips64 musl targets) - #143592 (UWP: link ntdll functions using raw-dylib) - #143595 (add `const_make_global`; err for `const_allocate` ptrs if didn't call) - #143678 (Added error for invalid char cast) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143829 (Trim `BorrowedCursor` API) - #143846 (pass --gc-sections if -Zexport-executable-symbols is enabled and improve tests) - #143851 (ci cleanup: rustdoc-gui-test now installs browser-ui-test) - #143856 (Linting public reexport of private dependencies) - #143895 (Dont collect assoc ty item bounds from trait where clause for host effect predicates) - #143922 (Improve path segment joining) - #143964 (Fix handling of SCRIPT_ARG in docker images) - #144016 (trait_sel: `MetaSized` always holds temporarily) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 15 pull requests Successful merges: - #142304 (tests: Add `RUST_BACKTRACE` and `-Cpanic` revisions to `panic-main.rs` test) - #143388 (Various refactors to the LTO handling code) - #143409 (Enable xgot feature for mips64 musl targets) - #143592 (UWP: link ntdll functions using raw-dylib) - #143595 (add `const_make_global`; err for `const_allocate` ptrs if didn't call) - #143678 (Added error for invalid char cast) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143829 (Trim `BorrowedCursor` API) - #143851 (ci cleanup: rustdoc-gui-test now installs browser-ui-test) - #143856 (Linting public reexport of private dependencies) - #143895 (Dont collect assoc ty item bounds from trait where clause for host effect predicates) - #143922 (Improve path segment joining) - #143964 (Fix handling of SCRIPT_ARG in docker images) - #144002 (Update poison.rs) - #144016 (trait_sel: `MetaSized` always holds temporarily) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143595 - fee1-dead-contrib:push-sylpykzkmynr, r=RalfJung,fee1-dead add `const_make_global`; err for `const_allocate` ptrs if didn't call Implements as discussed on Zulip: [#t-compiler/const-eval > const heap](https://rust-lang.zulipchat.com/#narrow/channel/146212-t-compiler.2Fconst-eval/topic/const.20heap/with/527125421) r? ```@rust-lang/wg-const-eval``` Fixes #129233
Implements as discussed on Zulip: #t-compiler/const-eval > const heap
r? @rust-lang/wg-const-eval
Fixes #129233